Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: update boxo to version with fewer globals #9907

Merged
merged 3 commits into from
Jun 8, 2023

Conversation

aschmahmann
Copy link
Contributor

@aschmahmann aschmahmann commented May 30, 2023

Validating ipfs/boxo#324 which collects the changes to resolve ipfs/boxo#291

@aschmahmann aschmahmann requested a review from a team as a code owner May 30, 2023 20:39
@aschmahmann aschmahmann force-pushed the feat/boxo-update-fewer-globals branch 2 times, most recently from 73e1ef4 to cfa056d Compare May 30, 2023 20:54
@@ -27,6 +27,8 @@ func dagImport(req *cmds.Request, res cmds.ResponseEmitter, env cmds.Environment
return err
}

blockDecoder := ipldlegacy.NewDecoder()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use a global here (i.e. the go-ipld-prime defaults) is pretty unfortunate and subtle. However, it should be ok.

Note: previously this decoder included the registration of the go-ipld-format Raw and Dag-Pb converters. However, IIUC that shouldn't be necessary in this function since the go-codec-dagpb codec and the built-in go-ipld-prime raw codec should be sufficient.

Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Meta boxo comment) It is sad to see a consumer go through breaking changes update for what is a code we want to kill.

@aschmahmann aschmahmann force-pushed the feat/boxo-update-fewer-globals branch from 624267d to 4cde204 Compare June 8, 2023 06:10
Copy link
Member

@hacdias hacdias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sgtm

@aschmahmann
Copy link
Contributor Author

A couple notes:

  1. rename dagpb protobuf to merkledag.v1 boxo#323 needs a review + merge first. Although this PR seems to validate that that one is fine.
  2. After the above PR is merged this one can be too, or it can be updated once the boxo release is cut depending on when that's happening.

@Jorropo Jorropo force-pushed the feat/boxo-update-fewer-globals branch from 4cde204 to 5fbe8c6 Compare June 8, 2023 07:30
@Jorropo Jorropo enabled auto-merge (rebase) June 8, 2023 07:30
@Jorropo Jorropo merged commit de59ac1 into master Jun 8, 2023
@Jorropo Jorropo deleted the feat/boxo-update-fewer-globals branch June 8, 2023 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

go-merkledag duplicates and global variables
3 participants